Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MARXAN - 291 Users should be able to upload shapefiles of scenario cost surfaces - update cost in database #206

Merged
merged 3 commits into from
May 26, 2021

Conversation

kgajowy
Copy link
Contributor

@kgajowy kgajowy commented May 24, 2021

image

commit (1) - adjust code definition to existing model; migrate cost as per @aagm request.
commit (2) - execute update

@vercel
Copy link

vercel bot commented May 24, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

marxan – ./app

🔍 Inspect: https://vercel.com/vizzuality1/marxan/6QvgscHB98LU84zrkn7pGKj2DL64
✅ Preview: https://marxan-git-api-marxan-410-cost-surface-update-db-vizzuality1.vercel.app

marxan-storybook – ./app

🔍 Inspect: https://vercel.com/vizzuality1/marxan-storybook/FMbzhVd1qxRiJPJytoLk453mQAMt
✅ Preview: https://marxan-storybo-git-api-marxan-410-cost-surface-update-db-fd778d.vercel.app

@kgajowy kgajowy force-pushed the api/MARXAN-410-cost-surface-update-db branch from 19cbee5 to 5c175c4 Compare May 24, 2021 12:09
@kgajowy kgajowy marked this pull request as ready for review May 24, 2021 12:23
@kgajowy kgajowy changed the title chore(geoprocessing): spud and spucd model update MARXAN - 291 Users should be able to upload shapefiles of scenario cost surfaces - update cost in database May 25, 2021
Comment on lines +50 to +53
puCostDataRepo.query(`
select spud.scenario_id, spucd."cost", spucd.output_results_data_id as pu_id from scenarios_pu_data as spud join scenarios_pu_cost_data as spucd on (spud."id" = spucd.scenarios_pu_data_id)
where spud.scenario_id = '${scenarioId}'
`),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the reason behind using a raw query?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a sake of simplicity and missing relations :(

Comment on lines +40 to +41
@RelationId((spud: ScenariosPutCostDataGeo) => spud.scenariosPlanningUnit)
scenariosPuDataId!: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if it will be handled correctly, AFAIR it was always an issue with RelationId and no Column decorator

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in af3773c

Copy link
Member

@hotzevzl hotzevzl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all looks ok - just a couple of minor things to cross-check

@@ -0,0 +1,59 @@
import { flatten, Injectable } from '@nestjs/common';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

though the end result is the same, I think this is a more-or-less internal TypeORM function that happens to be exported - should we use the lodash one for consistency?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in af3773c

`ALTER TABLE "scenarios_pu_cost_data" ADD "scenarios_pu_data_id" uuid`,
);
await queryRunner.query(
`ALTER TABLE "scenarios_pu_cost_data" ADD CONSTRAINT "FK_21454fad6e954ba771262974ae7" FOREIGN KEY ("scenarios_pu_data_id") REFERENCES "scenarios_pu_data"("id") ON DELETE NO ACTION ON UPDATE NO ACTION`,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I imagine you have discussed this with Alicia - just cross checking that the absence of cascades here is intentional. I can't think of a reason to leave cost data behind in case we delete scenario PUs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually I missed it - thanks for catching this out!
fixed in af3773c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants